encrypt bearer tokens at rest #302
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jbr
force-pushed
the
crypter
branch
7 times, most recently
from
d05023f
to
ad5e7ba
Compare
src/crypter.rs
Outdated
| let mut keys = s | ||
| .split(',') | ||
| .map(|s| { | ||
| STANDARD |
There was a problem hiding this comment.
Any reason not to use NO_PAD? (base64 padding is only necessary/useful if multiple b64 values need to be concatenated together, then parsed apart again; I don't think we need to do that.)
Less important, but any reason not to use the URL-safe alphabet?
There was a problem hiding this comment.
I have no opinion on this. I feel like this comes up every time we base64 something. I'm happy to switch it to URL_SAFE_NO_PAD or whatever is easiest for the generating side. Humans should never see these values so it doesn't matter how they're encoded
There was a problem hiding this comment.
Padding is not sufficient to allow concatenation of multiple base64-encoded values together. As a counterexample, note that AAAA is a valid encoding of three null bytes in all of base64, base64url, base64 without padding, and base64url without padding. Thus, it is not possible to unambiguously decode multiple concatenated encoded values from an input that consists of A repeated a multiple of four times.
RFC 4648 section 3.2 is very brief, but it seems to suggest that this is instead useful when the protocol employing base64 may be unclear about the "end of file".
Weighing in the other direction, base64 with padding is both recommended by RFC 4648 and better supported by tooling.
There was a problem hiding this comment.
addressed in 2a40071fb31f6d98bef09fe22b3fecc403f36025
There was a problem hiding this comment.
Interesting, the padding argument came from here, but the counterexample is clear. This makes it even less clear what the purpose of base64 padding is...
|
The sort of review I'm primarily looking for on this PR: Is my use of aes-gcm cryptographically correct/safe? Is it valid to concatenate the nonce and cipher text in a different order than Janus does? Is my implementation of rotation correct or is there a way of saving something about the key in the ciphertext so we can select the right key before trying to decrypt? |
|
Regarding key rotation: I think trial decryption is fine for our purposes for now, as we expect the number of keys will be around one to three, and the ciphertext will not be large. (for bearer tokens, it'll be a few dozen bytes, for other highly sensitive secrets, it'll likely be similar) There is prior art for specifying which key to use, but there's not much to it, just adding another fixed-length prefix before the nonce. DAP does this with the 8-bit |
jbr
force-pushed
the
crypter
branch
3 times, most recently
from
2260d22
to
eb1c32b
Compare
migration/src/m20230731_181722_rename_aggregator_bearer_token.rs
Outdated
Show resolved
Hide resolved
closes #215